Skip to content
This repository has been archived by the owner on Oct 30, 2024. It is now read-only.

✨ Use dynamic kubeclient #433

Merged
merged 4 commits into from
Jun 8, 2022
Merged

✨ Use dynamic kubeclient #433

merged 4 commits into from
Jun 8, 2022

Conversation

jerr
Copy link
Contributor

@jerr jerr commented May 30, 2022

Description

To be able to audit any type of resource a dynamic client is used instead of a clientset.

Type of change
  • Bug fix 🐛
  • New feature ✨
How Has This Been Tested?

All tests related to auditing resources from a cluster still work.
This features is used by the "deprecated apis" auditor (#428) to check all resources of a cluster.

Checklist:
  • I have 🎩 my changes (A 🎩 specifically includes pulling down changes, setting them up, and manually testing the changed features and potential side effects to make sure nothing is broken)
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • The test coverage did not decrease
  • I have signed the appropriate Contributor License Agreement

Copy link
Contributor

@genevieveluyt genevieveluyt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing! This is what I always wanted and never knew existed! Is there documentation that you use for discovery and dyanmic client or just the godoc? I have googled so many times for "how to unmarshal kubernetes objects from yaml in go" and never found it!!

My main concern is that we need the namespace resources, the rest are nits.

Comment on lines +145 to +146
// Namespace has to be included as a resource to audit if it is specified.
if apiresource.Name == "namespaces" && options.Namespace != "" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm understanding correctly, we need to treat namespaces differently because it's a Get instead of a List? We need the namespace resources for the netpol audit so I think we always want to include the namespace resource. If options.Namespace != "" then we only want to include the namespace resource where the namespace name is equal to options.Namespace, but it looks like kc.dynamicClient.Resource(gvr).Get(context.Background(), options.Namespace, metav1.GetOptions{}) takes care of that filtering?

Suggested change
// Namespace has to be included as a resource to audit if it is specified.
if apiresource.Name == "namespaces" && options.Namespace != "" {
// Namespace has to be included as a resource to audit if it is specified.
if apiresource.Name == "namespaces" {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the options.Namespace is not specified we want to retrieve all namespaces that is why we need to use the List function instead of the Get one.

With the options.Namespace != "" test we will use the Get will return the namespace according to the option otherwise the List will be used to return all namespaces.

If we use only the List that will return an empty unstructuredList if a namespace is specified:

unstructuredList, err := kc.dynamicClient.Resource(gvr).Namespace(options.Namespace).List(context.Background(), metav1.ListOptions{})

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohh ok that makes sense. Neat, thank you for the clarification!

@@ -80,25 +77,22 @@ func TestGetObjectMeta(t *testing.T) {
deployment := k8s.NewDeployment()
deployment.ObjectMeta = objectMeta
deployment.Spec.Template.ObjectMeta = podObjectMeta
assert.Equal(objectMeta, *k8s.GetObjectMeta(deployment))
assert.Equal(podObjectMeta, *k8s.GetPodObjectMeta(deployment))
assert.Equal(&objectMeta, k8s.GetObjectMeta(deployment))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity: this looks functionally equivalent, is there an advantage to doing it this way?

Copy link
Contributor Author

@jerr jerr Jun 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is because the k8s.GetObjectMeta function returns the metav1.Object interface instead of a metav1.ObjectMeta struct reference.

It is provided by the ObjectMetaAccessor.GetObjectMeta() function.

// GetObjectMeta returns the highest-level ObjectMeta
func GetObjectMeta(resource Resource) metav1.Object {
obj, _ := resource.(metav1.ObjectMetaAccessor)
if obj != nil {
return obj.GetObjectMeta()
}
return nil
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, missed the type switch and thought it was some go convention I didn't know about 😄

@jerr jerr force-pushed the use_dynamic_kuebclient branch from 1b4f526 to 6e25d12 Compare June 8, 2022 01:45
@jerr
Copy link
Contributor Author

jerr commented Jun 8, 2022

👆 rebase on the main branch to fix tests in CI.

@jerr jerr marked this pull request as ready for review June 8, 2022 11:59
@jerr jerr merged commit 81a1ceb into main Jun 8, 2022
@jerr jerr deleted the use_dynamic_kuebclient branch June 8, 2022 15:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants